fix(run): schema-force the plan turn and stop plan-file writes in hidden-write seats - #520
Merged
Merged
Conversation
…den-write seats The chef seat runs read-only, which for claude, cursor, and grok means a harness plan mode with every write tool hidden. The chef reached for the harness's own plan-file affordance, the write failed, and the failure tripped user-level hooks whose rebuttal prose replaced the plan JSON in the seat's final message. Both plan attempts recorded ok: true with parsed: false, and the run died on "orchestrator returned an invalid plan". Two changes. The corrective plan turn now restates the output contract next to the parse error instead of passing the error alone, so a seat mid-rebuttal has something to correct toward; retries stay bounded at one correction. And plan prompts for seats that launch without write tools say the plan lives in the reply and no file may be written, so the trigger never fires. A generic "do not modify files" rule did not cover the harness's own plan artifact. Closes #518 Co-authored-by: Claude <noreply@anthropic.com>
solomonneas
added
the
coderabbit-review
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesPlan-mode output safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant plan
participant agents.hides_write_tools
participant orchestrator
plan->>agents.hides_write_tools: determine read-only write-tool visibility
agents.hides_write_tools-->>plan: return no_file_writes
plan->>orchestrator: send plan prompt with applicable rules
orchestrator-->>plan: return plan response
plan->>plan: parse JSON plan
plan->>orchestrator: retry with PLAN_JSON_ONLY_RULE after parse failure
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_aboyeur.py (1)
3156-3183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the actual writable plan-mode and sandbox-override paths.
Line 3181 uses
codex, so it cannot catch accidental rule injection for writableclaude. Add a writable-claudecase and asandbox_read_only=Truecase to validate_orchestrator_hides_write_toolsintegration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_aboyeur.py` around lines 3156 - 3183, Update the plan-mode tests around test_plan_mode_orchestrator_is_told_not_to_write_a_plan_file and test_write_capable_orchestrator_keeps_the_plan_prompt_unchanged to exercise writable claude and sandbox_read_only=True scenarios. Add assertions that writable claude does not receive NO_PLAN_FILE_RULE, while sandbox_read_only=True causes the orchestrator’s write-tool hiding behavior and corresponding rule injection, validating _orchestrator_hides_write_tools integration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_aboyeur.py`:
- Around line 3156-3183: Update the plan-mode tests around
test_plan_mode_orchestrator_is_told_not_to_write_a_plan_file and
test_write_capable_orchestrator_keeps_the_plan_prompt_unchanged to exercise
writable claude and sandbox_read_only=True scenarios. Add assertions that
writable claude does not receive NO_PLAN_FILE_RULE, while sandbox_read_only=True
causes the orchestrator’s write-tool hiding behavior and corresponding rule
injection, validating _orchestrator_hides_write_tools integration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a125a67e-481f-4bd8-936f-d72db9935c97
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG*
📒 Files selected for processing (4)
src/brigade/aboyeur.pysrc/brigade/agents.pytests/test_aboyeur.pytests/test_agents.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #518
What happened
Two consecutive
brigade runfailures on the same task died withorchestrator returned an invalid plan: plan is not valid JSON. A third run in thesame session, same roster, different repo, planned and completed normally.
Repro evidence (run ids):
20260725-014242-7a7b15afand20260725-015900-d38906edfailed;
20260725-014248-48ba749eis the control.In both failures
plan-attempts.jsonrecordedok: true, parsed: falsefor both theinitial and the corrective turn: the seat call succeeded and the payload was prose.
The prose in both cases was the chef arguing with a harness memory-handoff gate, and
both texts reference a
Writeof a code-graph context file that failed because nowrite tool was enabled. The control run's chef never attempted that write.
Read-only claude, cursor, and grok seats launch in a harness plan mode with every
write tool hidden. Brigade never asked the chef to persist anything; the harness's own
plan-file affordance did. The failed write tripped user-level hooks, and the hook
rebuttal became the seat's final message.
The fix
Schema-force the plan turn. The corrective turn already existed but passed only
the raw parse error, which gives a seat mid-rebuttal nothing to correct toward. It now
restates the output contract next to the error: reply with the JSON plan object and
nothing else, no prose, no preamble, no tool-failure or hook commentary, and
{"assignments": []}if no worker is useful. Retries stay bounded at one correction,so a second prose turn still fails the run with the existing error rather than looping
a hijacked seat.
Remove the trigger.
agents.hides_write_tools()answers whether a seat launcheswithout file-write tools (claude, cursor, grok under read-only). When the orchestrator
is such a seat, the plan prompt says the plan lives in the reply and no plan, design,
or context file may be written. The existing read-only rule ("do not modify files")
did not cover the harness's own plan artifact.
Tests
plan-attempts.jsonrecordsinitial(unparsed, with the parse error) thencorrection(parsed), and the retryprompt carries both the parse error and the JSON-only contract.
orchestrator returned an invalid plan, twoseat calls, both attempts recorded.
plan prompt is unchanged.
hides_write_toolsunit coverage across plan-mode and write-capable seats.Verification
brigade work verify run --target . --command "./scripts/verify" --capture brigade-work-> completed, exit 0, 4203 passed / 3 skipped, coverage 82.67%.
Note for review
Worker and synthesis prompts run on the same plan-mode seats and could take the same
no-plan-file rule. Left out of scope here: only the plan turn parses the seat's final
message as structured output, so only the plan turn fails the run outright.
Summary by CodeRabbit
Bug Fixes
Tests